feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers#4441
feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers#4441waleedlatif1 wants to merge 12 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Extends MCP server CRUD to support Updates MCP client/service/connection-manager to use an SDK Reviewed by Cursor Bugbot for commit 8310051. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds spec-compliant OAuth 2.1 + PKCE support for outbound MCP servers, including auto-detection via an unauthenticated probe, a popup-based consent flow, encrypted per-server token persistence in a new
Confidence Score: 5/5The OAuth flow is safe to merge; all previously identified workspace boundary and token-management issues have been addressed. The implementation correctly handles SSRF validation before the auth-type probe, burns state before token exchange, verifies workspace ownership in both the callback and revocation paths, and encrypts all OAuth secrets at rest. The remaining nits are minor: a silent no-op on the discovery invalidation scope, a benign type mismatch in the refresh lock, and a low-probability TOCTOU UX issue where two concurrent users could overwrite each other's PKCE state. None of these affect correctness under normal usage. No files require special attention; all critical paths have been validated. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Browser (MCP Settings)
participant Start as /api/mcp/oauth/start
participant AS as Authorization Server
participant CB as /api/mcp/oauth/callback
participant DB as mcp_server_oauth (DB)
participant SDK as MCP SDK (mcpAuth)
UI->>Start: "GET ?serverId=&workspaceId="
Start->>DB: getOrCreateOauthRow(serverId, userId, workspaceId)
DB-->>Start: row
Start->>SDK: mcpAuth(provider, serverUrl)
SDK-->>Start: throw McpOauthRedirectRequired(authorizationUrl)
Start-->>UI: "{status: redirect, authorizationUrl}"
UI->>AS: window.open(authorizationUrl) [popup]
Note over UI,AS: User approves in popup
AS->>CB: "GET /callback?code=&state="
CB->>DB: loadOauthRowByState(hashState(state))
DB-->>CB: row (TTL-gated)
CB->>DB: clearState(row.id)
CB->>SDK: mcpAuth(provider, authorizationCode)
SDK->>AS: POST /token (PKCE code_verifier)
AS-->>SDK: "{access_token, refresh_token}"
SDK->>DB: saveTokens(row.id, tokens) [encrypted]
CB->>DB: clearVerifier(row.id)
CB-->>UI: HTML postMessage mcp-oauth, window.close
UI->>UI: invalidate server/tool queries
Reviews (30): Last reviewed commit: "fix(mcp): strip oauthClientSecret from o..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Greptile summary findings addressed in f587e82:
The point about clearing a pre-registered Client ID by emptying the field is a follow-up — |
|
@greptile |
|
@cursor review |
|
@greptile |
|
@cursor review |
|
@greptile |
|
@cursor review |
|
@greptile |
|
@cursor review |
|
@cursor review |
|
@greptile |
|
@greptile |
|
@cursor review |
…e check - withMcpOauthRefreshLock now attaches cleanup via .then(cleanup, cleanup) so cleanup is awaited as part of the lock chain and rejections don't produce unhandled promise rejection warnings. - revoke.ts: !res.ok already implies status !== 200; drop the redundant conjunction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
Look up the OAuth row by state once at the top of the callback so the serverId can be threaded into provider_error, missing_params, and unauthenticated closes. Without it, the popup's postMessage omits serverId and the parent UI can't clear the connecting state until the 500ms popup-closed poll fires. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
revokeMcpOauthTokens bailed when row.clientInformation was null, which is always the case for pre-registered clients (SimMcpOauthProvider.saveClientInformation is a no-op when preregistered is set). Fall back to mcpServers.oauthClientId so revocation proceeds for the pre-registered path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
- Burn the state row on the provider-error path so a replayed callback with the same state cannot complete a token exchange after the user saw the error. - Read the OAuth state row once at the top of the callback and reuse it on the happy path, eliminating a redundant query and a TTL-boundary race where a legitimate flow could be marked invalid_state between two reads. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@cursor review |
|
@greptile |
The useUpdateMcpServer optimistic update was spreading the raw updates object into the TanStack Query cache, which briefly placed the plaintext oauthClientSecret in client-side cache. Strip it from the optimistic spread — the cache holds the server-sanitized record after onSettled invalidation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8310051. Configure here.
| id: data.id, | ||
| workspaceId: variables.workspaceId, | ||
| name: variables.config.name, | ||
| transport: variables.config.transport, |
There was a problem hiding this comment.
Create mutation leaks plaintext secret in TanStack state
Medium Severity
The useCreateMcpServer mutation's mutationFn returns ...serverData which includes the plaintext oauthClientSecret from the user's input. TanStack Query persists this as mutation.data in its internal state. The PR author explicitly fixed the equivalent issue for useUpdateMcpServer by stripping oauthClientSecret from the optimistic update via const { oauthClientSecret: _omitSecret, ...safeUpdates } = updates, but the create mutation was missed. The plaintext secret remains accessible through React DevTools or TanStack Query state until the mutation is garbage collected.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8310051. Configure here.
| : null, | ||
| state: row.state, | ||
| updatedAt: row.updatedAt, | ||
| } |
There was a problem hiding this comment.
Duplicated OAuth row decryption/mapping logic across functions
Low Severity
loadOauthRow and loadOauthRowByState contain nearly identical ~20-line blocks that decrypt and map a raw DB row into an McpOauthRow. This duplicated decryption logic (for clientInformation, tokens, and codeVerifier) means a future fix or change to the mapping must be applied in both places, risking inconsistency.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8310051. Configure here.
1 participant
Summary
OAuthClientProviderWWW-Authenticate/oauth-protected-resource)mcp_server_oauthtable; SDK refreshes automatically before expiry/api/mcp/oauth/start→/api/mcp/oauth/callback) withstateCSRF protectionreauth_requiredfrom tool execution when refresh token is invalid so the UI can prompt to reconnectType of Change
Testing
Tested manually against OAuth-protected MCP servers (Linear). Existing header-auth servers regression-checked.
Checklist